Skip to content

Add MaxTxSize check to ValidateExpressLaneTx() #4105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
rauljordan merged 2 commits into from
Dec 11, 2025
Merged

Add MaxTxSize check to ValidateExpressLaneTx() #4105

rauljordan merged 2 commits into from
Dec 11, 2025
+158 −135

Conversation

@KolbyML
Copy link
Member

@KolbyML KolbyML commented Dec 3, 2025 •
edited
Loading

resolves NIT-4157

The premise is we should check if a transaction is valid, before we send it when possible.

We already check the MaxTxDataSize in ValidateTransaction(), so we should do it for ValidateExpressLaneTx() as well.

I am using uint64, as I don't think we should use int as it

  • varies in size depending on the platform
  • the parameter can't be negative

The sequencer takes length of msg.Transaction.MarshalBinary() for its internal size check, I am surprised it doesn't use .Size(), that is why I must do the same here.

@github-actions
Copy link

github-actions bot commented Dec 3, 2025 •
edited
Loading

❌ 4 Tests Failed:

Tests completed Failed Passed Skipped
2187 4 2183 0
View the top 3 failed tests by shortest run time 
TestMultiGasConstraintsStorage
Stack Traces | 0.000s run time 
=== RUN   TestMultiGasConstraintsStorage
=== PAUSE TestMultiGasConstraintsStorage
=== CONT  TestMultiGasConstraintsStorage
    constraints_test.go:377: 
        	Error Trace:	/home/runner/work/nitro/nitro/precompiles/constraints_test.go:377
        	Error:      	Not equal: 
        	            	expected: 0x1
        	            	actual  : 0x3
        	Test:       	TestMultiGasConstraintsStorage
--- FAIL: TestMultiGasConstraintsStorage (0.00s)

TestVersion30
Stack Traces | 6.090s run time 
... [CONTENT TRUNCATED: Keeping last 20 lines]
=== PAUSE TestVersion30
=== CONT  TestVersion30
    precompile_inclusion_test.go:94: goroutine 492097 [running]:
        runtime/debug.Stack()
        	/opt/hostedtoolcache/go/1.25.4/x64/src/runtime/debug/stack.go:26 +0x5e
        github.com/offchainlabs/nitro/util/testhelpers.RequireImpl({0x40ee4d0, 0xc051271a40}, {0x40abbe0, 0xc0db6d64e0}, {0x0, 0x0, 0x0})
        	/home/runner/work/nitro/nitro/util/testhelpers/testhelpers.go:29 +0x55
        github.com/offchainlabs/nitro/system_tests.Require(0xc051271a40, {0x40abbe0, 0xc0db6d64e0}, {0x0, 0x0, 0x0})
        	/home/runner/work/nitro/nitro/system_tests/common_test.go:1901 +0x5d
        github.com/offchainlabs/nitro/system_tests.testPrecompiles(0xc051271a40, 0x1e, {0xc0bf6addb0, 0x6, 0x0?})
        	/home/runner/work/nitro/nitro/system_tests/precompile_inclusion_test.go:94 +0x371
        github.com/offchainlabs/nitro/system_tests.TestVersion30(0xc051271a40?)
        	/home/runner/work/nitro/nitro/system_tests/precompile_inclusion_test.go:67 +0x798
        testing.tRunner(0xc051271a40, 0x3d2fe68)
        	/opt/hostedtoolcache/go/1.25.4/x64/src/testing/testing.go:1934 +0xea
        created by testing.(*T).Run in goroutine 1
        	/opt/hostedtoolcache/go/1.25.4/x64/src/testing/testing.go:1997 +0x465
        
    precompile_inclusion_test.go:94: �[31;1m [] execution aborted (timeout = 5s) �[0;0m
--- FAIL: TestVersion30 (6.09s)

TestVersion40
Stack Traces | 9.170s run time 
... [CONTENT TRUNCATED: Keeping last 20 lines]
�[90mwrite-args: deployed to 0xA02062aa4Cb0872315ACA3eA6dBF077d90B2DAC1�[0;0m
�[90mTime to activate write-args: 28.337029ms�[0;0m
    precompile_inclusion_test.go:94: goroutine 492098 [running]:
        runtime/debug.Stack()
        	/opt/hostedtoolcache/go/1.25.4/x64/src/runtime/debug/stack.go:26 +0x5e
        github.com/offchainlabs/nitro/util/testhelpers.RequireImpl({0x40ee4d0, 0xc051271c00}, {0x40abbe0, 0xc0db6d6120}, {0x0, 0x0, 0x0})
        	/home/runner/work/nitro/nitro/util/testhelpers/testhelpers.go:29 +0x55
        github.com/offchainlabs/nitro/system_tests.Require(0xc051271c00, {0x40abbe0, 0xc0db6d6120}, {0x0, 0x0, 0x0})
        	/home/runner/work/nitro/nitro/system_tests/common_test.go:1901 +0x5d
        github.com/offchainlabs/nitro/system_tests.testPrecompiles(0xc051271c00, 0x28, {0xc0a35f1df8, 0x5, 0x39?})
        	/home/runner/work/nitro/nitro/system_tests/precompile_inclusion_test.go:94 +0x371
        github.com/offchainlabs/nitro/system_tests.TestVersion40(0xc051271c00?)
        	/home/runner/work/nitro/nitro/system_tests/precompile_inclusion_test.go:71 +0x64b
        testing.tRunner(0xc051271c00, 0x3d2fe70)
        	/opt/hostedtoolcache/go/1.25.4/x64/src/testing/testing.go:1934 +0xea
        created by testing.(*T).Run in goroutine 1
        	/opt/hostedtoolcache/go/1.25.4/x64/src/testing/testing.go:1997 +0x465
        
    precompile_inclusion_test.go:94: �[31;1m [] execution aborted (timeout = 5s) �[0;0m
--- FAIL: TestVersion40 (9.17s)

📣 Thoughts on this report? Let Codecov know! | Powered by Codecov

@KolbyML KolbyML marked this pull request as draft December 3, 2025 20:56
@KolbyML KolbyML mentioned this pull request Dec 4, 2025
Closed
@KolbyML KolbyML force-pushed the NIT-4157 branch 3 times, most recently from 4559510 to 1715b85 Compare December 4, 2025 20:38
@KolbyML KolbyML marked this pull request as ready for review December 4, 2025 21:13
rauljordan
rauljordan reviewed Dec 4, 2025
View reviewed changes
cmd/el-proxy/config.go Outdated
}

func (c *ExpressLaneProxyConfig) Validate() error {
if c.MaxTxDataSize > arbostypes.MaxL2MessageSize-50000 {
Copy link
Contributor

@rauljordan rauljordan Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 50k is a magic number and will have issues if we specify a config smaller than it, maybe we can either abstract it as constant or move it to the other side of the inequality? Also add the values to the error format string

Copy link
Member Author

@KolbyML KolbyML Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I attempted to resolve the concern, let me know what you think

Copy link
Member

@pmikolajczyk41 pmikolajczyk41 Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the way you fixed it looks good, but I'd love to have a short comment above the constant why do we use 50'000 - I have no idea what this number represents actually 😅

Copy link
Member Author

@KolbyML KolbyML Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea as well, I added a comment with the details of when this change was added

@KolbyML KolbyML assigned rauljordan and unassigned pmikolajczyk41 Dec 4, 2025
@KolbyML KolbyML requested a review from rauljordan December 4, 2025 23:38
pmikolajczyk41
pmikolajczyk41 previously approved these changes Dec 5, 2025
View reviewed changes
Copy link
Member

@pmikolajczyk41 pmikolajczyk41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I have some minor questions, I'm happy with the current code, so an approval from my side ✅

ganeshvanahalli
ganeshvanahalli reviewed Dec 5, 2025
View reviewed changes
Copy link
Contributor

@ganeshvanahalli ganeshvanahalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do already have MaxTxSize check in the sequencer right?

nitro/execution/gethexec/sequencer.go

Lines 1295 to 1299 in 6837a8f

if queueItem.txSize > config.MaxTxDataSize {
// This tx is too large
queueItem.returnResult(txpool.ErrOversizedData)
continue
}

@KolbyML
Copy link
Member Author

KolbyML commented Dec 5, 2025

We do already have MaxTxSize check in the sequencer right?

nitro/execution/gethexec/sequencer.go

Lines 1295 to 1299 in 6837a8f

if queueItem.txSize > config.MaxTxDataSize {
// This tx is too large
queueItem.returnResult(txpool.ErrOversizedData)
continue
}

We do check for this in the sequencer, we should fail transactions before they reach the sequencer when possible.

pmikolajczyk41
pmikolajczyk41 previously approved these changes Dec 5, 2025
View reviewed changes
rauljordan
rauljordan previously approved these changes Dec 5, 2025
View reviewed changes
@rauljordan rauljordan enabled auto-merge December 5, 2025 17:28
ganeshvanahalli
ganeshvanahalli previously approved these changes Dec 6, 2025
View reviewed changes
@KolbyML
Copy link
Member Author

KolbyML commented Dec 10, 2025

@joshuacolvin0 helped show me Transaction.Size() and len(MarshalBinary) are the same thing. I updated the sequencer to use .Size(), along with the validation check I added.

@rauljordan rauljordan added this pull request to the merge queue Dec 11, 2025
Merged via the queue into master with commit f8bbec4 Dec 11, 2025
38 of 39 checks passed
@rauljordan rauljordan deleted the NIT-4157 branch December 11, 2025 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rauljordan rauljordan rauljordan approved these changes

@ganeshvanahalli ganeshvanahalli ganeshvanahalli left review comments

@pmikolajczyk41 pmikolajczyk41 pmikolajczyk41 left review comments

Assignees

@joshuacolvin0 joshuacolvin0

Labels

design-approved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@KolbyML @rauljordan @ganeshvanahalli @pmikolajczyk41 @joshuacolvin0